feat: Prevent LDAP sync from adding users to abac rooms/teams #37299
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThis PR adds ABAC (Attribute-Based Access Control) guards to user-channel and user-team synchronization in LDAP Manager, preventing sync operations from adding users to channels or teams that have ABAC-protected rooms. It introduces a new finder method to query private rooms with ABAC attributes. Changes
Sequence DiagramsequenceDiagram
participant LDAPSync as LDAP Sync
participant RoomsModel as Rooms Model
participant DB as Database
rect rgb(230, 240, 255)
Note over LDAPSync,DB: User-Channel/Team Sync Flow
alt ABAC Enabled
LDAPSync->>LDAPSync: Check if ABAC enabled
activate LDAPSync
LDAPSync->>RoomsModel: findPrivateRoomsByIdsWithAbacAttributes()
activate RoomsModel
RoomsModel->>DB: Query: _id in ids, type='p', abacAttributes exists
DB-->>RoomsModel: ABAC-protected room IDs
RoomsModel-->>LDAPSync: ABAC room results
deactivate RoomsModel
LDAPSync->>LDAPSync: Filter out ABAC rooms from candidates
alt Rooms/Teams Remain
LDAPSync->>LDAPSync: Add to non-ABAC channels/teams
else No Rooms/Teams Remain
LDAPSync->>LDAPSync: Return early (skip operation)
end
deactivate LDAPSync
else ABAC Disabled
LDAPSync->>LDAPSync: Proceed with normal sync
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span three related files with straightforward logic additions: new model method, its interface definition, and LDAP manager integration. The code patterns are consistent and logic density is moderate, though reviewers should verify ABAC filtering correctness and ensure the early-return conditions are handled properly. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37299 +/- ##
=============================================
+ Coverage 70.09% 70.11% +0.02%
=============================================
Files 3032 3032
Lines 103981 103981
Branches 18491 18494 +3
=============================================
+ Hits 72887 72909 +22
+ Misses 29144 29118 -26
- Partials 1950 1954 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/ee/server/lib/ldap/Manager.ts(2 hunks)packages/model-typings/src/models/IRoomsModel.ts(1 hunks)packages/models/src/models/Rooms.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/models/src/models/Rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
packages/model-typings/src/models/IRoomsModel.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (4/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (3/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (4/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (3/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (2/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (1/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (1/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (2/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (1/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (3/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (2/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (3/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (1/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (4/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (4/4) - Alpine (Official)
- GitHub Check: 🔨 Test API (EE) / MongoDB 5.0 (1/1) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (2/5) - Alpine (Official)
🔇 Additional comments (3)
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
361-364: LGTM - ABAC guard correctly prevents channel auto-join.The check appropriately skips adding users to ABAC-protected channels during LDAP sync. The error logging provides clear feedback about why the channel was skipped.
packages/model-typings/src/models/IRoomsModel.ts (1)
220-220: LGTM - Type signature correctly defines the new ABAC-aware finder method.The method signature properly describes the new finder for private rooms with ABAC attributes, aligning with the implementation in the Rooms model.
packages/models/src/models/Rooms.ts (1)
1271-1279: LGTM - Implementation correctly queries private rooms with ABAC attributes.The method implementation properly filters for:
- Rooms matching the provided IDs
- Private room type (
'p')- Non-empty
abacAttributesarrayThe query structure and use of
$existswith$ne: []ensures only rooms with actual ABAC configuration are returned.Note: This method is scoped to private rooms only. If the use case requires checking public rooms (type
'c') for ABAC attributes as well, consider creating a more general method or renaming this to clarify its scope.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-55
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes